-
Notifications
You must be signed in to change notification settings - Fork 2
Closes #3: add unit tests for PeriodAxis #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closes #3: add unit tests for PeriodAxis #4
Conversation
| period_axis = PeriodAxis.from_series(series_date) | ||
| assert period_axis.between(np.datetime64("2023-01-02"), np.datetime64("2023-01-03")).tolist() == [1] | ||
| assert period_axis.between(np.datetime64("2023-01-02"), np.datetime64("2023-01-03"), closed=Closed.BOTH).tolist() == [1, 2] | ||
| assert period_axis.between(np.datetime64("2023-01-02"), np.datetime64("2023-01-03"), closed=Closed.RIGHT).tolist() == [2] No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also we need general linting and formatting like last time :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I added a pre-commit dependency and defined formatting and type check as the steps in the pre-commit. I'd love to show you but I currently can't commit because it is failing type checks lol. But let me know if you think if it's a good idea, once you get back to me on the closed parameter issue, and I push the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've seen some integrated CIs running in GitHub actions that gets auto run and tagged onto the commit/PR. that issue you're dealing with is the exact reason i'd be hesitant to add a pre-commit hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi James, please have a look at the pre-commit yaml file, let me know if you think the steps are okay to include.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like exactly what we should run as pre-commit, check out the checks i added for #7. i'll merge these pre-commit changes along with that.
i'll just need you to 1. create a new branch with only the pre-commit changes, 2. amend the pre-commit changes off of this commit, and then i can incorporate that commit into #7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The branch I created is called: add-pre-commit-steps, but it only has pre-commit and github CI actions. Let me know if I misunderstood you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! that's all, it's merged now
5325d35 to
7954ad2
Compare
7954ad2 to
6963f22
Compare
|
nice! this brings us up to about 40% coverage overall already |
No description provided.